Repository navigation
Conversation
bcf6733 to
b2ed1d9
Compare
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
I marked this PR draft after a second correctness review. Deferring writeback alone is insufficient: the current write-watch policy preserves whole 64 KiB blocks, which can discard the draw output in the same block as a post-draw CPU store. Also, a writeback recorded during a completion needs to execute synchronously rather than remain in a newly opened recorder batch. I am working through a region-aware writeback path and will update the PR when the regression and completion ordering are addressed. |
|
Update after checking the reporter’s run on current |
|
I pushed two commits to #8246: |
fogydes
left a comment
There was a problem hiding this comment.
Reviewed the diff, CI, and verified the existing test on current main.
Production code (Draw.cpp): The fix defers the resident linear color target writeback from draw-recording time to batch completion via recorder.OnComplete(). Before this change, StorageTexture::FlushPending ran synchronously during recording — scheduling a whole-target copyout before any CPU stores the game makes after Draw returns. At completion, the OnComplete lambda:
- Acquires
GuestMemory::GpuMutex()— correct, sinceFlushPendingandWriteBackmutate shared guest-memory state. - Flushes pending overlapping aliases (preserving prior alias reconciliation behavior).
- Calls
keep->WriteBack()to write the GPU results back.
The keep shared_ptr capture extends the resident's lifetime until the completion callback runs, preventing a use-after-free if the texture is otherwise released. This is sound.
Test (LinearTargetResident.cpp): The regression sequence is well-designed:
Program()now takes ablueConstantparameter (default0x80), allowing the second draw to write a distinct blue value (0xf4).Draw()now takesscissorWidth(defaultWidth), allowing a left-scissored second draw.- The
runlambda executes two draws: full-width blue=0x80, then left-half blue=0xf4. Between draws, theseedvariant writes0x5ato the left half of the CPU buffer. After both draws, the CPU writes0x42to the right half. AfterWaitIdle(), it checks:- Left half: second draw's output (
x, y, 2.0, 1.0with0xf4blue → float2.0viav_mov_b32encoding) preserved. - Right half: CPU-written
0x42bytes preserved (not overwritten by GPU writeback).
- Left half: second draw's output (
- Both
run(true)(seeded) andrun(false)(unseeded) are exercised.
The instruction encoding 0x7e000200u | blueConstant | (22u << 17u) is a v_mov_b32 with an inline constant — the constant index formula is correct for GCN/RDNA ISA.
CI: conventions, labels pass. Build/platform jobs skip (AGC change, no CI GPU runner).
Local verification: Built agc_driver_linear_target_resident_tests and ran ctest -R "^agc_driver_linear_target_resident$" -V with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 on current main — Passed (0.56s, NVIDIA GeForce RTX 5050 Laptop GPU). The test passes on the existing code; the enhanced test from this PR would exercise the new writeback path.
PR body: Honest about the hardware limitation (Xeon E5-1620 v2 can't run Vulkan tests). Template complete.
One observation: the PR doesn't note VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation testing, but the host CPU guard prevents the runtime from reaching Vulkan initialization anyway, so this is consistent with the limitation.
No blocking issues found.
|
Follow-up: I found the first byte-merge revision could compare against an upload baseline that was stale across recorded draws. Commit |
|
Thanks for the detailed review and for checking the previous code on your RTX 5050. Since that review, the fix changed: it now leaves target results pending after batch completion and stores them on the later read/flush path; commit |
|
Sharing a tested alternative implementation and regression coverage for #8152 so we can coordinate with this PR. Branch: rajvveer/AnyPS5 — fix/agc-linear-target-cpu-writes This approach captures the actual image on the GPU before a recorded linear draw and merges only changed bytes into guest memory. Atomic word updates preserve untouched channels sharing a word. Image and guest baselines remain separate for subsequent refresh; CPU write-back and padded array layers are covered too. It keeps the recorded path. The tests extend Celegans12's reproducer: seeded/unseeded partial draws, masked float/RGBA8 channels, partial geometry, later blending, queued overlapping draws, actual cached-recipe replay, and GPU retile cases for 1/2/4/8/16-byte elements. Recipe replay is unseeded so a deferred re-upload cannot mask the original bug. Validation: Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release with SPIRV-Tools; NVIDIA RTX 5070 (617.42) and AMD Radeon(TM) Graphics (32.0.21030.2001).
These results apply to the linked branch; I have not executed this PR's current Would incorporating the implementation or regression coverage here be useful, or would you prefer a coordinated alternative PR? I have kept the separate PR on hold. |
50f477f to
e32a19b
Compare
|
I chose to integrate your implementation and regression commits into #8246 rather than open a second PR for the same issue. Both commits remain authored by you ( |
|
Correction to my previous note: I said I would keep the PR draft open, but GitHub shows it was already marked ready for review at 07:46 UTC. I had the state wrong; I did not change the draft/ready setting as part of integrating your commits. The PR remains ready for review, with the current-head runtime rerun still outstanding. |
|
The requested exact-head rerun is complete: Hardware: Windows 11 Pro 26200, NVIDIA GeForce RTX 5070 / driver 617.42, and AMD Radeon(TM) Graphics / driver 32.0.21030.2001; GCC 15.2.0 Release with SPIRV-Tools. The matrix was five resident modes (default, unwatched, draw transitions, eager write-back, synchronous linear targets) plus cached-recipe replay, three repetitions on each GPU. Every process confirmed the intended GPU and loaded Khronos validation layer, with A small test-only follow-up is also available: 66f62d174 — strengthen linear target replay and array coverage. It changes only
The follow-up was separately built and tested at Please cherry-pick |
|
I rebuilt exact head Attached is a tested correction extending Rajveer Shekhawat’s implementation and MarcZX10’s integration. The seeded/unseeded regression derives from Celegans12’s reproduction. I’m offering this for integration into the existing PR. Confirmed failures and correctionThe concurrent regression uses a 64 × 4096 RGBA8 target. The GPU writes only blue while a CPU thread updates counters in red/green and checks the preceding values. On GPU word-level compare-and-swap does not provide atomicity with ordinary CPU stores to adjacent bytes. The correction uses a separate publication shader with write-only A second regression exposed a stale-baseline problem: an actual GPU write equal to stale image contents can be omitted by the before/after comparison. The comparison is not treated as a write-coverage detector. It depends on establishing a coherent baseline first. Unsynchronized CPU/GPU stores to the same byte still require ordering. Publication remains in the recorded command stream, with the existing reader/host barriers and resource retention. The new checks use the established TestedHardware: NVIDIA GeForce RTX 4070 Laptop GPU, NVIDIA 592.82 / Windows driver 32.0.15.9282, Windows 11 Pro 10.0.26300. WinLibs GCC 15.2.0 r7, Release, Ninja; Khronos validation layer 1.4.363.0.
Every GPU run used: Actual validation-layer loading was verified. Base and correction logs contain zero VUIDs, synchronization hazards, missing-layer warnings or skips. The shared deprecated-setting notice is not counted as a new validation error. Focused candidate command: ctest --test-dir build-review-8152 -R '^agc_driver_(linear_target_(concurrent_bytes|cpu_writes.*|publication.*|resident|recipe)|graphics|alias_copy_label|array_layer_target)$' -V --timeout 30Convention and whitespace checks pass. The attachment is a plain unified diff against SHA256: The additional upload/snapshot costs work and temporary memory. No game compatibility, performance improvement, Linux/macOS runtime, fresh full-suite or other-GPU result is claimed for this correction. AI-assisted: yes. |
f063d00 to
030409a
Compare
|
Integrated the offered correction into #8246 as commit The contributor-reported RTX 4070 Laptop run and correction-tree test results are recorded in the PR description, explicitly as pre-latest-rebase results. On the rebased head, |
|
This is a runtime run of the exact current head Setup: Windows 11 (26300), WinLibs GCC 15.2.0, Release,
Not measured: the cost per draw. AI-assisted: yes (Claude Code); results above were run on this machine. |
|
I tested #8246 with Ghost of Yōtei (PPSA26344) on Windows 11 and an RTX 3080 Laptop, since #6892's resident linear targets are what broke our warm world runs in the first place. The unit-level fix holds up well on our tree. In the game, though, it doesn't fix the spawn: with resident linear targets on, Atsu still isn't at the river. One caveat first: this isn't plain main. Our tree is main b509767 merged with our local Yōtei work ( The tests all pass. Your new tests, the upstream resident-linear tests and the nearby storage and write-tracking tests went 22/22, three times each, with resident linear on. My #8152 reproducer passes 5/5 in both the seeded and the unseeded case. With only your Draw.cpp hooks taken out again, it fails 3/3 with 7,680 of the 8,192 right-half bytes rolled back, as before. In the game, the warm run from our Ainu-village save never shows Atsu. From frame 078 on, the camera sits under the water looking up at the forest. That's the same family of symptoms as our earlier resident-linear runs (T-pose in the sky). With resident linear off, the same save spawns him at the river every time. The path does run there: in the world, about 15-43 recorded draws per 10 s go into resident linear targets, and each one gets your "linear draw baseline" upload. I also wanted to know whether the game just reads those targets before the recorded write-back lands. So I added a local debug switch that does a full If it helps, I can run an instrumented build next. For example, it could dump a linear target's guest bytes after one of these draws with resident linear on and off and diff them. Or I can try a newer head. Is there anything you'd want me to log in particular? Details
|
|
@Celegans12, thanks for offering. A targeted capture on the current head |
|
Thanks for the detailed title run and the sync control. That changes the next question: the isolated CPU-write regressions pass, but this title still diverges even when each writeback is forced to complete before the next packet, so completion ordering alone does not explain the spawn failure. The most useful next capture would be the first target/draw that differs between resident-linear on and off: guest address, dimensions/format/pitch, bytes before the draw, resident image bytes after it, bytes written back, and the next consumer’s read. A narrow capture around the first divergence should distinguish stale baseline/image contents from a format/layout or alias mismatch. If you can share such a trace (or an instrumented reproducer), I can investigate the next fix in this PR. |
|
Thanks, this separates the #8152 byte-preservation case from the remaining Yōtei spawn failure: the focused CPU-write/repro tests and their negative control still validate the intended fix, while the title remains below water even with the per-draw sync switch. For the first affected target, could you log a compact identity (guest address, format, extent/pitch) and a hash plus a few sampled bytes at three points: after |
|
Thanks for the detailed run. The result narrows this down: the reproducer confirms the CPU write-back regression is fixed, while forcing a full sync after each recorded draw does not fix the Yōtei scene, so I agree we should look at the bytes and resident image contents rather than add more ordering waits. If you can run one instrumented comparison, please pick a target touched during frames 078–090 and log/dump:
Please capture resident-on and resident-off runs from the same save and report the first differing byte/pixel with the owning draw. A small bounded set around one target is enough; no need for a full frame dump. That should tell us whether the mismatch is in the write-back, the resident image used by the next draw, or another local Yōtei change. |
The detiler test checks that an ordinary detile has no baseline, that a changed-only dispatch binds the original with a sub-word offset, reuses one pipeline across baseline offsets and that ordinary retiling afterwards is unchanged. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
PrepareLinearDraw and FinishLinearDraw, and the buffer a linear draw keeps its original bytes in. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
PrepareLinearDraw writes back and flushes what is pending, uploads the target, then copies the image into a buffer that holds the bytes the draw started from next to a baseline copy; the recorder keeps the buffer and counts its bytes. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
FinishLinearDraw flushes the target's pending store and drops the snapshot once no layer is pending. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…ass starts recordDraw calls PrepareLinearDraw for every target, ahead of StartDrawPass. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
recordPreparedDraw does the same for the targets it binds when the draw is recorded. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
The completion store of a drawn linear target goes through FinishLinearDraw. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
Write-back compares the 64 KiB blocks written since the draw's snapshot and leaves the ones the CPU changed. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
A write-back that used the draw's snapshot publishes the whole shadow, not the partial units. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
The write-back detiles into the buffer the draw started from instead of a new one and keeps it with its size. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…napshot With a snapshot the write-back runs the changed-only detile against the draw's original bytes and no longer copies the tiled scratch over the import. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
A copy of the destination is taken before the store and the bytes the write-back did not change are put back, so concurrent CPU bytes survive. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…napshot main stores a drawn linear target through storeDrawnRect, which copies the whole drawn rectangle over the import and so overwrites the bytes of channels the draw masked out that the CPU wrote since. A target with a draw snapshot already stores only the bytes that changed, so writeBackLayers takes the rectangle path only when there is no snapshot.
Resources, the detiler and the standard headers it now uses. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…rget A left-half geometry and a program whose blue constant can be chosen. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…k and vertices The draw helper can halve the destination with blending, restrict the color write mask and the scissor, and take its own vertices. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…in the resident test The resident test gets a Coverage mode, the APS5_NO_RESIDENT_LINEAR flag, checks of queued and cached draws and of the bytes a draw leaves alone, a CPU snapshot check, a changed-only retile check and a --recipe mode that replays the draw through a draw recipe. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
agc_driver_linear_target_recipe runs agc_driver_linear_target_resident_tests --recipe. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
Draws into a linear target while the CPU writes other bytes of it and checks that the CPU's bytes are kept. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
Six cases (mask, geometry, reader, eviction, alias, same) check what a resident linear target publishes after a draw; registered per case. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
A CPU-write regression with two switches, APS5_LINEAR_SAME_OUTPUT and APS5_LINEAR_SEED_LEFT, and a seeded registration; the switches are documented in ENVIRONMENT.md. Co-authored-by: Rajveer Shekhawat <rajveershekhawat626@gmail.com> Co-authored-by: embersas <30363137+embersas@users.noreply.github.com>
…he whole target The write-back of the previous draw leaves the image current, so the forced whole-surface upload before every recorded draw copied bytes the image already held. Refresh uploads only the layers whose guest memory changed since the image was last current, which includes the CPU's writes.
9377004 to
8868994
Compare
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (3, not in the main baseline's validation pass)Validation IDs already present on main (13 IDs in 554 tests)Skipped (48)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
# Conflicts: # core/libs/prx/libSceAgcDriver/Graphics/src/Texture.cpp
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (3, not in the main baseline's validation pass)Validation IDs already present on main (9 IDs in 593 tests)Skipped (57)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the rebase. Against the last review, using @savvaniss's rerun on 9377004c3c (same commit as the perf(agc) one on this head; later pushes are merges of main):
- Memory is still unbounded per draw: device-local usage grows about 71 MiB per recorded draw against about 24 MiB on base (12,901 MiB vs 4,333 MiB at 180 draws in one frame; peak private commit 13.2 GiB vs 4.6 GiB). Please bound the snapshots (release or reuse them per draw, or cap how many are live) and post peak device-local memory before/after at 60 and 180 draws, see the memory section of CONTRIBUTING.
- New stall: at 180 draws per frame the slowest single draw is 315 to 397 ms in every run (5,036 ms once at 150), against 7 to 10 ms on base. It looks like waiting on the frame's queued work while device-local memory is near 11 to 13 GiB. Please find what triggers it; the memory bound above may be the fix.
agc_driver_linear_target_publication_aliasfailed withwrong CPU bytes=557056on9377004c3con both AMD (write-watched) and NVIDIA. The NVIDIA run on the current head passes with no new validation IDs. Please say what fixed it, and rerun it on a write-watched host (Windows) to confirm.- First-frame cost: savvaniss measured about +170 ms in frame 0 (mostly in
WaitIdle) and later frames waiting about half as long as base. Please say what the first frame pays for. - Minor: the
Testedline still says the validation run onmainis not posted; update it with the results above.

What
Fixes #8152: a later write-back from a recorded linear-target draw could overwrite CPU stores made after
Drawreturned.This PR uses the coordinated implementation authored by Rajveer Shekhawat (two commits) and the concurrent-byte correction authored by embersas. The original three commits were split into a series of smaller ones, so each commit derived from their work carries
Co-authored-bytrailers for both; the drawn-rectangle fix (leave the drawn-rectangle store to targets without a draw snapshot) and the finalRefresh()commit are mine and carry none. It snapshots the resident image around a recorded linear draw and merges changed bytes through the linear detiler so untouched guest bytes and CPU writes survive. The regression coverage includes seeded and unseeded partial draws, masked channels, partial geometry, later blending, queued overlapping draws, cached-recipe replay and GPU detiles for supported element widths.This is separate from #1191's resident-target performance path and #7154's depth-plane buffer coherence. It does not depend on either PR.
Tested
Current head
db12f5d6fb(30 commits, rebased ontomain3fea2a937b):agc_driver_linear_target_*tests plusagc_driver_graphics: 22 pass and 2 skip on lavapipe (direct_write_completion,import_creation). WithANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationthere are 0VUID-lines (the layer is inserted at instance and device level). The same run onmainis not posted yet.main, flat in the number of draws and targets (one live snapshot per draw). Per-draw cost was measured only with the guest memory not write-watched (kernel 6.1, no write watch), where the target is compared or uploaded whole: about 44 ms onmainand 74 to 99 ms with this PR depending on whether the CPU writes before each draw. No number from a write-watched host and notools/trace_digest.pysummary are available yet. Details are in the comments below.python3 tools/check_conventions.py --base upstream/mainandgit diff --checkpass.Earlier reports, on older heads and not repeated on this one:
e5b9ac546against base3fd1cc393, the author reports 36/36 fresh focused runs passed on NVIDIA RTX 5070 (driver 617.42) and AMD Radeon(TM) Graphics (driver 32.0.21030.2001), Windows 11 Pro 26200, GCC 15.2 Release with SPIRV-Tools and Khronos validation/synchronization validation enabled, with no new VUIDs or validation warnings, and 118/118 common selected tests per GPU on both base and change.fea0c0cd8a2e8f85454e0d4b5b917915e0e3035b), 15/15 focused tests and 20/20 repeated key-case executions on a Windows 11 RTX 4070 Laptop, NVIDIA 592.82, with Khronos synchronization validation. The concurrent regression recorded 5,668 lost CPU updates on the then-current head and the original unseeded control onmainlost 7,680 of 8,192 CPU-written bytes; the corrected cases passed 20/20 repeats with zero lost updates.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)e5b9ac546); NVIDIA RTX 4070 Laptop / driver 592.82 on Windows 11 Pro (reported by Embersas on an older correction tree)